refactor(membership): one callable spelling, no dead member shapes, and arms built from the real document (TASK-170) - #1949
Merged
Conversation
…nd arms built from the real document (TASK-170)
Three traps that produced the TASK-165/166 membership defects, closed.
(1) `utils/isPodMember` exported the creator-inclusive rule three ways — the
default, the named `isPodMember`, and the strict rule beside them — so the
obvious spelling `require('./utils/isPodMember')` bound the permissive one;
TASK-165's M2 mutation is that edit, and it stayed green. Deleted rather than
renamed: the creator clause answered "did `Pod.members` forget this creator",
which the pre-save hook already covers, and a live-looking function with no
caller is one import away from being reached for again. The module now exports
`{ isListedPodMember }` and nothing callable by default, asserted in a new
unit suite whose first arm is a positive control on the export object.
(2) `getRecap`, `getUserFeed` and `getDecisionHistory` each carried a second
spelling of membership (`{ 'members.userId': … }`, `{ 'members._id': … }`) that
matched nothing — no pod has either shape (Vera: 0 of 424) — while teaching that
a member is an object with a key. That belief is what produced the `getPodFeed`
defect TASK-166 fixed, so the terms go, along with the same arm in
`getDecisionHistory`'s post-filter: with the query narrowed, an object-shaped
member cannot reach that filter, and a reader that admits what the selector does
not select is the worse of the two.
(3) TASK-166's two `leavePod` arms were built on `members: ['creator','member']`,
so `[...pod.members].includes(req.userId)` — a reasonable-looking tightening —
would keep both green while refusing every real member: spreading unwraps
mongoose's array wrapper into plain ObjectIds, where `pod.members.includes(hex)`
casts. The arms now build the pod from the real model, with `isMongooseArray` as
the shape control and a fixture-control arm asserting the difference they depend
on. M6 in the ledger is that spread edit, and it now reddens the control arm.
One test fixture was load-bearing on the dead shape and is disclosed rather than
quietly repaired: `attentionQueue`'s durable-read arm mocked a pod whose member
was `{ userId: 'member-1' }` — a pod that cannot exist — and narrowing the
post-filter turned it red with "Access denied". That is the belief living in the
corpus, which is the cleanest evidence for (2)'s premise.
…d export beside it (TASK-170)
Found by Wren (74715). `services/connectorRelayPolicy.ts` said the permissive
`isPodMember` "is deliberately NOT imported any more" and that the strict rule is
"Defined beside `isPodMember` in utils" — true when it was written, false as of
this PR, where nothing sits beside it.
Comment-only, so the six witnesses Vera ran at `15f550a1` stand unchanged; the
diff from that head is this one file.
Swept for the same stale claim rather than fixing the one site: the remaining
references to `isPodMember` in `backend/` are either historical ("Before TASK-161
this read the permissive `isPodMember`"), a different local helper
(`pgMessageController`'s `isPodMemberInMongo`), or the AX audit entry that
records this exact name collision as history — all accurate. The two plan docs
under `docs/plans/` name `isPodMember` as the predicate a gate *should* check;
they are dated design records, and the strict rule is what such a gate uses, so
they are left alone (flagged to Wren rather than changed unilaterally).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cut from
b95ec1eb(#1945's merge). Backend only, no version bump. Three rows' worth of traps that produced the TASK-165/166 membership defects, closed at the source rather than defended site by site.(1) One callable spelling
utils/isPodMemberexported the creator-inclusive rule three ways —module.exports = isPodMember, a namedisPodMember, and the strictisListedPodMemberbeside them. So the obvious spelling bound the permissive rule:TASK-165's M2 mutation is exactly that edit, and it stayed green. After #1945 nothing in production called it: 8 binders, every one destructuring
isListedPodMember— re-verified here rather than taken from the row:and zero bare
isPodMember(calls elsewhere inbackend/.Deleted, not renamed. The creator clause answered "did
Pod.membersforget this creator" — a creation-time false negative thatPod'spre('save')hook already covers. A renamed function with no caller is one import away from being reached for again, and the old name carries no information a comment cannot. The module exports{ isListedPodMember }; the docstring records what was removed and why.New suite
__tests__/unit/utils/isPodMember.test.js(6 arms). Its first arm is the one the row asks for, and it is an absence — so the instrument is the export object, with a positive control so a brokenrequirecannot pass for a deletion:The file had no dedicated suite before this, which is its own gap: the one rule every membership decision flows through was covered only through its callers.
(2) The dead member shapes
getRecapandgetUserFeedqueried{ 'members.userId': userId }beside{ members: userId };getDecisionHistoryadditionally kept'members._id'. Vera's census: 0 of 424 pods carry amembers.userIdentry and none has a non-ObjectId member, so the terms selected nothing.They are not merely inert. They encode a member is an object with a key, which is the belief behind the
getPodFeeddefect #1945 fixed — that reader accepted a nested shape on a pod list that is a flat array of ObjectIds, and refused every caller. The terms go.One judgement beyond the row, flagged for the gate.
getDecisionHistory's post-filter also carriedmember?.userIdin its||chain. I removed that arm too: with the query narrowed, an object-shaped member can no longer reach the filter, so leaving it would make the post-filter the one place that still admits what the selector does not select. The live arms (member?._id, and the bare-ObjectId case) stay, and two arms cover it — a pod whose member is{ userId }is refused, and a member carrying_idstill passes.A test fixture was load-bearing on the dead shape, disclosed rather than quietly repaired.
activityService.attentionQueue.test.js's "reads settled decisions durably for a current pod member" mockedmembers: [{ userId: 'member-1' }]— a pod that cannot exist — and narrowing the post-filter turned it red withAccess denied. That is the belief living in the corpus, and it is the cleanest evidence for the row's premise: the wrong shape was not only in the query, it was in the fixtures that were supposed to be checking the query.(3) The arms are built from the real document
TASK-166's two
leavePodarms usedmembers: ['creator', 'member'].leavePoddecides withpod.members.includes(req.userId), and on a hydrated document mongoose's array wrapper casts the hex string, so it matches. Rewrite it as[...pod.members].includes(req.userId)— exactly the kind of tightening a reviewer asks for — and it refuses every member, because spreading unwraps the wrapper into plain ObjectIds. With string members both spellings agree, so the arms could not see that edit. Wren caught this; it is the general form of the fixture-shape rule #1947 is putting inTESTING.md.Both arms now build the pod from the real model (
jest.requireActual,doc.save/doc.populatestubbed and nothing else), plus a fixture-control arm that asserts the shape the arms depend on:The third line is a fact about mongoose, not about
leavePod— it is there so a future reader can see why the fixture cannot be a string array, and it fails loudly if the fixture is ever "simplified" back.Ledger — 9 mutations, each alone, no survivors
exports no callable default…exports no callable default…refuses a creator who is not listedand TASK-166'sfails closed when the pod's creator has left it_idarm removed from the ruleadmits a member document carrying '_id'members.userIdrestored ingetRecapbuilds the viewer's pod list from membership…members.userIdrestored ingetUserFeedselects the viewer's pods by membership…members._idrestored ingetDecisionHistoryreads settled history by membership onlymember?.userIdarm restored in the post-filterdoes not admit a pod whose member is the legacy '{ userId }' shapeleavePoddecides through a spread instead of the wrapperleavePod still removes a non-creator member (control)M6 is the one this PR exists for: on
mainthat edit is green. It reddens the control arm and not the 409 arm, which is the expected shape — the guard returns before the membership check.M1c reddening an arm from a different suite (TASK-166's) is worth noting: the strict rule is exercised through both its own suite and
podController's, so the deletion is witnessed from two directions.Runs and lint
activityService.*×5,podController,isPodMember): 8 suites / 69 tests green.__tests__/unit— 426 suites / 3909 tests green (425/3903 onb95ec1eb; +1 suite/+6 arms from the new file).__tests__/service+__tests__/services— 36 suites / 433 tests green, 24 skipped (real-DB tier)..ts: 0 errors, 0 warnings on any touched line,fatalchecked..jstest corpus errors disclosed as ever (the un-gatedimport/no-unresolved+import/extensionsclass).Gate: Vera.
Gate follow-ups
connectorRelayPolicy's comment (Wren, 74715) — fixed at6efd9ac9. The block said the permissiveisPodMember"is deliberately NOT imported any more" and that the strict rule is "Defined besideisPodMemberin utils" — true when written, false as of this PR. It now says the home module carries no creator-inclusive export, and that the creator clause is covered at creation byPod's pre-save hook rather than by a second predicate.Comment-only, so the six witnesses run at
15f550a1stand; the diff from that head is one file, comment lines only. I swept for the same claim rather than fixing the one site: the otherisPodMemberreferences inbackend/are historical ("Before TASK-161 this read the permissiveisPodMember"), a different local helper (pgMessageController'sisPodMemberInMongo), or the AX-audit entry recording this exact name collision — all still accurate. The twodocs/plans/docs nameisPodMemberas the predicate a gate should check; they are dated design records and the strict rule is what such a gate uses, so they are untouched, flagged rather than changed unilaterally.The
member?._idresidual, kept deliberately (Vera, 74714). After narrowing the post-filter,member?._idsurvives in it. With.lean()and no.populate()it cannot fire either — the same class one step less obviously. It stays because it is the branchisListedPodMemberitself keeps: removing it here would make this reader and the shared predicate disagree about which member shapes exist, and the whole point of this PR is that there is one definition. The refusals that matter are witnessed instead — a pod whose member is{ userId }is refused, and a member carrying_idstill passes.Status: cleared by Vera at
15f550a1; head is6efd9ac9(comment-only), so the re-check is a diff.